Add Timed Transcripts Editor. #1111

polesye · 2013-09-24T15:43:18Z

This PR adds timed transcripts editor functionality for video player in studio.

Design wireframes for this PR is on wiki

Documentation of transcripts workflow is placed inside this PR to developers documentations folder.
Please navigate to sphinx generated developers documentation -> CMS->Transcripts

@Lyla, @frrrances, @singingwolfboy please review.

You can play with it on Blades sandbox at http://studio.sandbox-blades-001.m.edx.org/edit/i4x://private/05/vertical/f1d269cde2784fa99cd5e0f3a9604d62 .

Rollback functionality is not part of this PR.

auraz · 2013-09-24T16:08:35Z

@wedaly All tests are passed locally, but on Jenkins I see corrupted gridfs messages. Can you help with this please?

wedaly · 2013-09-24T16:23:26Z

@auraz This error means there is a Mongo resource conflict, most likely with the contentstore database. You'll need to: (1) randomize the name of the Mongo database used as a contentstore, (2) drop and recreate the database before/after each test.

See cms/djangoapps/contentstore/tests/test_contentstore.py for the correct way to do this. The important lines are:

TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE)
TEST_DATA_CONTENTSTORE['OPTIONS']['db'] = 'test_xcontent_%s' % uuid4().hex

@override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE, MODULESTORE=TEST_MODULESTORE)
class ContentStoreToyCourseTest(ModuleStoreTestCase):

and

def tearDown(self):
    MongoClient().drop_database(TEST_DATA_CONTENTSTORE['OPTIONS']['db'])
    _CONTENTSTORE.clear()

Longer term, we're working on building an abstraction for this (much like we do with ModuleStoreTestCase). I'm also setting up a Jenkins cluster, to avoid having multiple tests using the same database.

auraz · 2013-09-25T07:03:20Z

@wedaly thank you.

Lyla-Fischer · 2013-10-01T12:48:25Z

@singingwolfboy @nedbat I'm going to be bugging you in person a bit later, but I wanted to remind you that this pull request exists. I know it is a monster, but..... It adds important functionality and only you can let this into the codebase
http://www.fs.usda.gov/Internet/FSE_MEDIA/stelprdb5362178.jpg

singingwolfboy · 2013-10-01T13:20:11Z

cms/djangoapps/contentstore/features/transcripts.py

+
+
+@step('I run ipdb')
+def run_ipdb(_step):


If we want to include a step like this in our codebase, it should probably be in common/djangoapps/terrain/steps.py. Alternatively, we may just want to remove it entirely.

singingwolfboy · 2013-10-16T15:50:24Z

cms/static/js/views/transcripts/metadata_videolist.js

+                this.messenger
+                    .render('not_found')
+                    .showError(
+                        'No sources',


showError method use gettext inside.

polesye · 2013-10-17T15:53:37Z

@singingwolfboy , @nedbat All your comments are addressed, please continue. :)

nedbat · 2013-10-18T20:51:14Z

I'm all done reviewing. 👍

singingwolfboy · 2013-10-21T15:10:20Z

cms/djangoapps/contentstore/tests/test_transcripts_utils.py

+        """)
+
+        with self.assertRaises(transcripts_utils.TranscriptsGenerationException) as cm:
+                transcripts_utils.generate_subs_from_source(youtube_subs, 'BAD_FORMAT', srt_filedata, self.course)


This line seems indented strangely.

indent is fixed.

singingwolfboy · 2013-10-21T15:54:16Z

The only remaining issue that I'm concerned about is the nested require call in cms/static/js/views/metadata.js. Once that's fixed, then I'm satisfied and I can give my 👍

Lyla-Fischer · 2013-10-22T07:49:00Z

👍

singingwolfboy · 2013-10-22T14:16:21Z

cms/static/js/views/metadata.js

@@ -32,6 +32,11 @@ define(["backbone", "underscore", "js/models/metadata"], function(Backbone, _, M
                    else if(model.getType() === MetadataModel.LIST_TYPE) {
                        new Metadata.List(data);
                    }
+                    else if(model.getType() === MetadataModel.VIDEO_LIST_TYPE) {
+                        var VideoList = require("js/views/transcripts/metadata_videolist");


You need to add "js/views/transcripts/metadata_videolist" to the list of modules in the define call at the top of this file, and bind it to the VideoList variable there. Is there a reason that you want to call require here, instead of getting the module at the top of the file?

@singingwolfboy because I have circular dependencies (See, metadata_videolist.js:5 and metadata_videolist.js:39).
If I do so, it breaks a code. And I have fixed it according to Circular Dependencies and Sugar;
And it works okay.

Ah, OK. I thought that you still needed to include the module name in the define call at the top of the file, and you would just get passed a null value until you use require inside the function to get the true value. But if this works, then I'm OK with it. (It would be nice if this code could be restructured to avoid the circular dependency, but I don't know if that's possible.)

To do so Metadata.AbstractEditor (that is stored in the js/views/metadata.js now) should be moved to separate file. In this case, circular dependency will be avoided.

Is that all? Can you do that in this pull request?

@singingwolfboy Done. Please check 69e3978

singingwolfboy · 2013-10-22T18:23:06Z

Awesome. 👍 once the tests pass.

Add Timed Transcripts Editor.

YONK-997: api-integration version bump

ghost assigned nedbat Sep 24, 2013

singingwolfboy reviewed Oct 1, 2013
View reviewed changes

singingwolfboy reviewed Oct 16, 2013
View reviewed changes

singingwolfboy reviewed Oct 21, 2013
View reviewed changes

singingwolfboy reviewed Oct 22, 2013
View reviewed changes

polesye and others added 3 commits October 22, 2013 21:45

Add Timed Transcripts Editor.

aecc20a

Fix bug. Add validation to HTML5 sources field.

ba5f337

Add special cases to doc.

ee7c718

polesye added a commit that referenced this pull request Oct 23, 2013

Merge pull request #1111 from edx/anton/video_subtitles

6c08708

Add Timed Transcripts Editor.

polesye merged commit 6c08708 into master Oct 23, 2013

polesye deleted the anton/video_subtitles branch October 23, 2013 09:38

pomegranited pushed a commit to open-craft/edx-platform that referenced this pull request Aug 8, 2018

Merge pull request openedx#1111 from edx-solutions/aamishbaloch/YONK-997

a351d32

YONK-997: api-integration version bump

baby636 mentioned this pull request Nov 5, 2022

[Snyk] Fix for 1 vulnerabilities baby636/edx-platform#33

Open

baby636 mentioned this pull request Dec 26, 2022

[Snyk] Fix for 1 vulnerabilities baby636/edx-platform#41

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Timed Transcripts Editor. #1111

Add Timed Transcripts Editor. #1111

polesye commented Sep 24, 2013

auraz commented Sep 24, 2013

wedaly commented Sep 24, 2013

auraz commented Sep 25, 2013

Lyla-Fischer commented Oct 1, 2013

singingwolfboy Oct 1, 2013

auraz Oct 4, 2013

singingwolfboy Oct 16, 2013

polesye Oct 16, 2013

polesye commented Oct 17, 2013

nedbat commented Oct 18, 2013

singingwolfboy Oct 21, 2013

polesye Oct 22, 2013

singingwolfboy commented Oct 21, 2013

Lyla-Fischer commented Oct 22, 2013

singingwolfboy Oct 22, 2013

polesye Oct 22, 2013

singingwolfboy Oct 22, 2013

polesye Oct 22, 2013

singingwolfboy Oct 22, 2013

polesye Oct 22, 2013

singingwolfboy commented Oct 22, 2013

Add Timed Transcripts Editor. #1111

Add Timed Transcripts Editor. #1111

Conversation

polesye commented Sep 24, 2013

auraz commented Sep 24, 2013

wedaly commented Sep 24, 2013

auraz commented Sep 25, 2013

Lyla-Fischer commented Oct 1, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polesye commented Oct 17, 2013

nedbat commented Oct 18, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singingwolfboy commented Oct 21, 2013

Lyla-Fischer commented Oct 22, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singingwolfboy commented Oct 22, 2013